Prevent DNS rebinding attack on admin routes #108
Conversation
|
@randomstuff, could you please have a look at this PR? |
| // for proxying an external URL. E.g. Request-Line (RFC 7230, Section 3.1.1) | ||
| // has no scheme. | ||
| // Serve local admin routes when the `Host` is well-known, e.g. `[hostname]:[port]`, | ||
| // `hetty.proxy`, `localhost:[port]` or the listen addr `[host]:[port]`. | ||
| return strings.EqualFold(host, hostname) || | ||
| req.Host == "hetty.proxy" || |
There was a problem hiding this comment.
I was going to say that it would theoretically be possible to use a DNS rebinding attack on http.proxy assuming the attacker is in position of MITM. However, as the port is not included it won't work in normal deployments AFAIU and this might not be a real problem. So I believe this is OK as long as the application is not deployed on a standard port.
There was a problem hiding this comment.
It's not clear to me however, whether this is really useful (?).
|
Look OK to me. |
| return strings.EqualFold(host, hostname) || | ||
| req.Host == "hetty.proxy" || | ||
| req.Host == fmt.Sprintf("%v:%v", "localhost", listenPort) || | ||
| req.Host == fmt.Sprintf("%v:%v", listenHost, listenPort) || | ||
| req.Method != http.MethodConnect && !strings.HasPrefix(req.RequestURI, "http://") | ||
| req.Host == fmt.Sprintf("%v:%v", listenHost, listenPort) |
There was a problem hiding this comment.
Does it work if listenHost == ""? Alternatively, allowing anything of the form {rawIpv4}, {rawIpv4}:{port}, {rawIpv6}, {rawIpv6}:{port}` should be OK with respect to DNS rebinding.
There was a problem hiding this comment.
For example if listenHost == "0.0.0.0", the socket will be available to all IP addresses of the host. However, the server will only accept requests using 0.0.0.0:{port} which is what we want in this case.
There was a problem hiding this comment.
I'm wondering whether something along those lines would be OK:
host, port, splitErr := net.SplitHostPort(req.Host)
// ...
return ... ||
ParseIP(req.Host) == nil ||
(splitErr != nil && ParseIP(host) == nil && IsInteger(port);
| // for proxying an external URL. E.g. Request-Line (RFC 7230, Section 3.1.1) | ||
| // has no scheme. | ||
| // Serve local admin routes when the `Host` is well-known, e.g. `[hostname]:[port]`, | ||
| // `hetty.proxy`, `localhost:[port]` or the listen addr `[host]:[port]`. | ||
| return strings.EqualFold(host, hostname) || |
There was a problem hiding this comment.
DNS rebinding through this might theoretically be possible :
- when hetty is not bound to localhost;
- through a browser on another host which can reach the hetty instance (eg. on the same LAN).
But this might not be the most standard use case and the most compelling attack.
With this change, DNS rebinding attacks on the admin routes should no longer be possible, and result in a
502 Bad Gatewayresponse.To test: